Skip to content

fix(preprocessor): enforce Zend enum declaration rules - #61

Open
AlessioGiacobbe wants to merge 6 commits into
swoole:masterfrom
AlessioGiacobbe:split/enum-declaration-rules
Open

fix(preprocessor): enforce Zend enum declaration rules#61
AlessioGiacobbe wants to merge 6 commits into
swoole:masterfrom
AlessioGiacobbe:split/enum-declaration-rules

Conversation

@AlessioGiacobbe

Copy link
Copy Markdown
Contributor

Enum declaration rules were entirely unenforced — all of these compiled (each a Zend compile fatal, probed for the exact rule and message): properties in enums; the forbidden magic-method set (__construct, __destruct, __clone, __get/__set/__unset/__isset, __sleep/__wakeup, __set_state, __serialize/__unserialize, __toString, __debugInfo — the full set probed one by one; __call/__callStatic/__invoke stay legal); a case with a value in a non-backed enum and a case without one in a backed enum; duplicate case names and case/const clashes; backing types other than int|string; explicit implements UnitEnum/BackedEnum; abstract methods in enum bodies.

Enum ClassDef flags now also carry FINAL (mirroring ZEND_ACC_FINAL on enum class entries), so class B extends SomeEnum is rejected by the existing final-class check with Zend's own wording, and final-class devirtualization legitimately applies to enums.

Deliberately not added, because Zend evaluates them lazily at runtime: case-value/backing-type mismatches and duplicate case values.

Part of the split of #39.

@matyhtf matyhtf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two enum paths are still outside these checks.

  1. Forbidden magic methods introduced by a trait are accepted because the new check only runs in prepareClassMethod(). Composed and aliased trait methods are installed through installComposedTraitMethod():
trait T { public function __construct() {} }
enum Suit { use T; case Hearts; }

Zend reports Enum Suit cannot include magic method __construct; this PR dry-compiles it. Centralize the enum magic-method check and call it for composed/aliased methods as well. Add at least a trait-injected __construct test (an alias renamed to a forbidden magic name should follow the same path).

  1. Backed enum case values are not limited to scalar literal nodes. case Two = 1 + 1 and constant references are valid PHP, but this code still reads $v->expr?->value during prepare:
enum Number: int { case Two = 1 + 1; }
function main(): void { var_dump(Number::Two->value); }

TypePHP emits Undefined property: BinaryOp\Plus::$value, records null, and later resolves the backed case as the string case name instead of integer 2. Preserve the expression AST in prepare and evaluate it in the convert phase using the existing constant-expression machinery; do not assume a scalar node or evaluate implementation expressions during prepare. Add runtime coverage for an arithmetic expression and a constant reference.

@AlessioGiacobbe
AlessioGiacobbe force-pushed the split/enum-declaration-rules branch from c4a161a to 36fd7bb Compare September 2, 2026 08:21
@AlessioGiacobbe

Copy link
Copy Markdown
Contributor Author

Both paths closed, rebased on current master:

  1. Trait-composed magic methods — the ban is centralized in assertEnumMayIncludeMethod() and now also runs where composed/aliased trait methods are installed, which sees post-adaptation names: both a trait-declared __construct and use T { foo as __construct; } fail with Zend's message ("Enum Suit cannot include magic method __construct"). Tests for both shapes.

  2. Non-literal backed case values — prepare now stores scalar literals directly and keeps any other expression as its AST (no evaluation during prepare, no Throwable catching, no null overloading); the constant machinery evaluates it lazily on first convert-phase access and memoizes. Plain constant references resolve too, so const TWO = 2; case Two = TWO; works. Number::Two->value folds to int(2) instead of the case-name string, and the registration emits ZVAL_LONG(..., 2) (it always had the correct value — the compile-time folder was the broken half). Runtime PHPT covers arithmetic, constant references, a constant product, string concat, ->name, and from(), with Zend-validated expectations.

The rebase also surfaced a pleasant interaction: upstream's abstract-method checks now fire before the enum complaint for abstract private function in enums, matching Zend's diagnostic order (probed).

@AlessioGiacobbe
AlessioGiacobbe force-pushed the split/enum-declaration-rules branch 2 times, most recently from c4576f3 to bef4331 Compare September 2, 2026 10:21

@matyhtf matyhtf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase this branch onto the latest master; #45, #52, #59, #63, and #66 have now been merged.

Two constant-expression paths still need correction.

  1. enumCaseExprs is unset only after evaluation succeeds, but there is no in-progress/visiting guard. Accessing a self-referencing case such as enum E: int { case A = E::A; } recursively enters getClassConstValue() until the compiler exhausts the stack/memory. Zend reports Cannot declare self-referencing constant E::A. Mutual cycles need the same protection.
  2. Lazy evaluation can run while the translator is converting a different namespace. getClassConstValue() currently prefers $this->namespace, and the class-constant callback reduces Name\FullyQualified to a string, so an enum expression declared in namespace A may be resolved relative to namespace B when first referenced there.

Please add an evaluation stack keyed by the declaring enum/case, preserve the AST until successful completion, and resolve names using the expression declaration context/resolved AST name rather than the current conversion namespace. Add self-cycle, mutual-cycle, and cross-file/cross-namespace tests.

@AlessioGiacobbe
AlessioGiacobbe force-pushed the split/enum-declaration-rules branch from bef4331 to f679874 Compare September 2, 2026 13:18
@AlessioGiacobbe

Copy link
Copy Markdown
Contributor Author

Rebased onto current master. Both paths fixed:

  1. Cycle guard — lazy evaluation now runs inside an in-progress set keyed by Enum\Fqn::CaseName; re-entry emits Cannot declare self-referencing constant \E::A`, and the AST stays in place until an evaluation completes successfully (popped in a finally). Probed against Zend 8.4.13 to match the reported constant exactly: E::Aforcase A = E::A;, E::Bfor the mutual paircase A = E::B; case B = E::A;` regardless of which case is accessed, and likewise for a 3-cycle.
  2. Declaration-context resolution — the class-constant callback now prefers the resolved AST name (resolvedName attribute / the node's fully qualified form), passed with a leading backslash so the active conversion namespace can never be prepended on top; and the preprocessor captures the declaring file's use-imports into ClassDef::$enumUse* (mirroring the existing traitUse* pattern), with evaluation running inside withDeclarationNameContext() — factored out of withTraitNameContext(), which now delegates to it.

Tests: self-cycle and mutual-cycle fatals with the exact messages, plus cross-namespace (with a decoy same-name symbol in the referencing namespace) and cross-file positives asserting the generated case/constant values, with the referencing file converted first.

@matyhtf matyhtf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two compatibility gaps remain.

  1. A valid backed-enum constant expression using an enum property fetch still fails:
enum E: int {
    case One = 1;
    case Two = E::One->value + 1;
}

Zend PHP accepts this and resolves E::Two->value to 2. This branch fails in the registration path because the independent constant-expression evaluator in gen_stub.php rejects Expr_PropertyFetch. Please either teach that evaluator about enum-case ->value and ->name, or pass the value already resolved by preprocessing into stub registration.

  1. The cycle-detection key currently lowercases both the enum name and case name:
$key = strtolower($enumName . "::" . $name);

Enum class names are case-insensitive, but enum case names are case-sensitive; PHP permits both case A and case a. Use the equivalent of:

$key = strtolower($enumName) . "::" . $name;

Please add coverage for enum property fetches in backed values and for distinct A / a cases, including a dependency between their values.

Zend rejects at compile time, and TypePHP previously accepted silently:
- properties in enums (instance, static, hooked): enum class entries
  have no property table ("Enum E cannot include properties")
- magic methods other than __call/__callStatic/__invoke ("Enum E
  cannot include magic method __x"); the banned set was probed one by
  one against Zend 8.4.13
- a case value on a non-backed enum and a missing value on a backed
  enum ("Case A of ... enum E must (not) have a value")
- duplicate case names and case/const name collisions: enum cases are
  class constants ("Cannot redefine class constant E::A")
- a backing type other than int|string
- explicitly implementing UnitEnum/BackedEnum, which Zend adds itself
  ("cannot implement previously implemented interface"), including the
  non-backed-enum BackedEnum variant
- abstract methods in enum bodies: an enum can never be abstract

Enum ClassDef flags now carry Modifiers::FINAL, mirroring ZEND_ACC_FINAL
on enum class entries, so `class B extends E` is rejected by the
existing final-class inheritance check without touching the Translator.
…sions

Two gaps against Zend 8.4 in the enum declaration rules:

- The forbidden-magic-method check only ran in prepareClassMethod(), so a
  magic method arriving through a trait — either declared there or created
  by an alias adaptation that renames an ordinary method to a magic name —
  was silently accepted. Zend applies the ban to every method installed in
  the enum ("Enum Suit cannot include magic method __construct", probed on
  8.4.13 for both the composed and the aliased form). The check is now
  centralized in assertEnumMayIncludeMethod() and also invoked from
  installComposedTraitMethod(), which sees post-adaptation names.

- A backed case value beyond a scalar literal (case Two = 1 + 1, or a
  constant reference) read the nonexistent ->value off the expression node,
  warning "Undefined property" and recording null, which later made the
  constant folder resolve Number::Two to the case-name string. Zend accepts
  any constant expression here. The preprocessor now keeps the expression
  AST (the symbol environment is incomplete during prepare) in a ClassDef
  case-name => Expr map, and the convert phase evaluates it lazily with the
  existing constant-expression machinery in ClassConstantValueTrait,
  memoizing the result on first access. Plain constant fetches now also
  resolve program constants recorded by parseConstDef(), so
  `const TWO = 2; ... case Two = TWO;` folds to 2.
…oreign contexts

Two constant-expression paths in the lazy backed-case evaluation were
still incorrect:

- No in-progress guard: a self-referencing case such as
  `enum E: int { case A = E::A; }` re-entered getClassConstValue()
  through the stored expression AST until the stack was exhausted, and
  mutual cycles (`case A = E::B; case B = E::A;`) did the same. The
  evaluation now marks the case in progress for its duration
  (mirroring CONST_RECURSIVE on a Zend class-constant fetch, with
  gen_stub's case-table evaluation playing Zend's unmarked outer
  access) and fails when a marked case is fetched again, reporting the
  same constant Zend names on 8.4.13: `E::A` for the self-cycle and
  `E::B` for the mutual cycle. The AST entry now survives until
  evaluation succeeds, so an aborted evaluation cannot leave a
  half-initialized null behind in enumCases.

- Wrong resolution context: the first fetch of a case may happen while
  the translator is converting a different file (a constant initializer
  in namespace B referencing A\E::X), and names inside the stored
  expression were resolved against that context: the ClassConstFetch
  callback reduced the name node to a bare string, which
  getClassConstValue() then prefixed with the active namespace. The
  callback now prefers the NameResolver's resolvedName attribute (or
  the node's own fully qualified form), and the evaluation runs inside
  the enum's declaration context — its namespace plus the declaring
  file's use tables, captured at prepare like the trait ones — through
  withDeclarationNameContext(), factored out of withTraitNameContext()
  which needed the identical swap.

gen_stub's processStubFile() wrapped every exception into a bare
RuntimeException; TestError now passes through so compile diagnostics
raised during stub generation keep their type for the test harness.

Tests cover the self-cycle and mutual-cycle fatals (messages probed on
Zend 8.4.13), a cross-namespace program with a decoy B\Helper constant,
and a cross-file pair converted referencing-file-first, asserting the
zvals emitted into the stub registration (values verified against Zend
8.4.13).
…base

Since the enum-case class-constant work merged, `const REF = \A\E::X`
registers the case object as a persistent enum-case AST — matching Zend,
where var_dump(\B\Holder::REF) prints enum(A\E::X) — instead of folding
to the backing scalar. The cross-context tests now assert that
registration; the enum's own backed value stays a ZVAL_LONG.
Two compatibility gaps against Zend 8.4 in backed enum case values:

- A case value fetching another case's property
  (`case Two = E::One->value + 1;`) failed at stub registration: the
  independent constant-expression evaluator in gen_stub.php rejects
  Expr_PropertyFetch. The constant-expression machinery in
  ClassConstantValueTrait now evaluates `Case->value` / `Case->name`
  (PHP 8.2 "fetch properties in const expressions": the object must
  itself be a constant expression evaluating to an enum case; property
  names stay case-sensitive), and EnumCaseInfo::getDeclaration()
  consumes the value resolved there for any non-literal case value
  instead of re-evaluating the AST independently, so one evaluator owns
  cycle detection and declaration-context name resolution. The
  registration entry resolves through the unguarded evaluation, keeping
  Zend's cycle reporting: the mutual cycle still names E::B, and a true
  cycle through a property fetch (`case A = G::A->value + 1;`) fails
  with "Cannot declare self-referencing constant G::A" (probed on
  8.4.13).

- The cycle-detection key lowercased the whole "Enum::Case" string, but
  only enum class names are case-insensitive: `case A` and `case a` are
  distinct cases, and resolving a constant that walks both at once
  (`const REF = F::A;` with `case A = F::a->value - 1; case a = 2 + 1;`)
  reported a false "self-referencing constant F::a". The key now
  lowercases only the enum name.

Tests cover a property-fetch backed value (int arithmetic on ->value and
string concatenation on ->name), distinct A/a cases with a dependency
between their values held in the in-progress guard simultaneously, and
the self-cycle through ->value; all fixtures verified against Zend
8.4.13.
@AlessioGiacobbe
AlessioGiacobbe force-pushed the split/enum-declaration-rules branch from f679874 to 9a8bd06 Compare September 3, 2026 08:33
@AlessioGiacobbe

Copy link
Copy Markdown
Contributor Author

Rebased onto current master; both gaps addressed.

  1. Went with your second option — registration consumes the value the front-end already resolved — plus the prerequisite it implies: the front-end constant-expression evaluator also rejected Expr_PropertyFetch, so it now resolves EnumClass::Case->value / ->name (object must itself be a constant expression evaluating to an enum case; property names case-sensitive). EnumCaseInfo carries the enum FQCN and getDeclaration() substitutes the translator-resolved scalar for any non-literal value expression before EvaluatedValue::createFromExpression, so gen_stub keeps a single source of truth instead of a second evaluator that could diverge. Cycle-reporting fidelity is preserved: registration enters unguarded (Zend's case fetch does not mark CONST_RECURSIVE either; only nested constant fetches mark), which keeps the mutual-cycle report at E::B for both access orders — re-probed on 8.4.13.

  2. The cycle key now lowercases only the enum name: strtolower($enumName) . '::' . $name. Proven live — with the old key restored, the new fixture fails with a false self-reference on F::a.

Tests (Zend-verified): E::One->value + 1ZVAL_LONG(&enum_case_Two_value, 2) (plus a string-backed ->name concatenation case); distinct A/a cases with a derived from A->value and a forward reference between them; self-reference through ->value reporting Cannot declare self-referencing constant G::A as Zend does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants